Merged
Conversation
…irty store - add app.json filetype for app identification - tidy
🎭 Playwright: ✅ 547 passed, 0 failed · 6 flaky📊 Browser Reports
|
Contributor
📝 WalkthroughWalkthroughAdds workflow-aware filename utilities (.app.json support), replaces the builder "save" flow with a "default view" feature (removing save-success UI and useBuilderSave), updates workflow save/overwrite logic to be mode-aware, and moves linear-mode selection persistence into active workflow extra.linearData. Changes
Sequence DiagramsequenceDiagram
actor User
participant BuilderToolbar as BuilderToolbar
participant Hook as useAppSetDefaultView
participant Dialog as DefaultViewDialog
participant WorkflowStore as WorkflowStore
participant WorkflowService as WorkflowService
User->>BuilderToolbar: Click "Set default view"
BuilderToolbar->>Hook: showDialog()
Hook->>Dialog: open(initialOpenAsApp)
User->>Dialog: Choose view -> Apply
Dialog->>Hook: emit('apply', openAsApp)
Hook->>WorkflowStore: update activeWorkflow.initialMode\nand app.rootGraph.extra.linearMode
Hook->>WorkflowService: persist/rename as needed
WorkflowService->>WorkflowStore: persist changes / emit toasts
Hook->>Dialog: close()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎨 Storybook: ✅ Built — View Storybook |
📦 Bundle: 4.48 MB gzip 🟢 -544 BDetailsSummary
Category Glance App Entry Points — 17.9 kB (baseline 17.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 976 kB (baseline 980 kB) • 🟢 -4.08 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 72.1 kB (baseline 72.1 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 436 kB (baseline 436 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 736 B (baseline 736 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 46.9 kB (baseline 46.9 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.64 MB (baseline 2.64 MB) • 🔴 +752 BStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 55.5 kB (baseline 55.5 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 12 added / 12 removed Vendor & Third-Party — 8.84 MB (baseline 8.84 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.89 MB (baseline 7.89 MB) • 🟢 -153 BBundles that do not match a named category
Status: 56 added / 56 removed |
⚡ Performance Report
Raw data{
"timestamp": "2026-03-03T13:24:39.238Z",
"gitSha": "a4ed56777112c54583e4c31c00d7b17f841dddc8",
"branch": "pysssss/app-mode-data-saving-refactor",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2033.1959999999754,
"styleRecalcs": 126,
"styleRecalcDurationMs": 27.67,
"layouts": 1,
"layoutDurationMs": 0.18400000000000014,
"taskDurationMs": 442.5350000000001,
"heapDeltaBytes": -3178616
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2080.454999999972,
"styleRecalcs": 184,
"styleRecalcDurationMs": 52.87599999999999,
"layouts": 12,
"layoutDurationMs": 2.988,
"taskDurationMs": 1070.827,
"heapDeltaBytes": -2359596
},
{
"name": "dom-widget-clipping",
"durationMs": 594.2120000000273,
"styleRecalcs": 43,
"styleRecalcDurationMs": 13.328,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 387.04499999999996,
"heapDeltaBytes": 7020640
}
]
} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/workflow/core/services/workflowService.ts (1)
157-191:⚠️ Potential issue | 🟡 MinorPropagate save/cancel outcome from
saveWorkflowto callers.For temporary workflows,
saveWorkflowAs()can returnfalse(cancel), butsaveWorkflow()currently returnsvoid. That prevents caller flows from distinguishing cancel vs success.Proposed fix
- const saveWorkflow = async (workflow: ComfyWorkflow) => { + const saveWorkflow = async (workflow: ComfyWorkflow): Promise<boolean> => { if (workflow.isTemporary) { - await saveWorkflowAs(workflow) + return await saveWorkflowAs(workflow) } else { syncLinearMode(workflow, [app.rootGraph]) workflow.changeTracker?.checkState() @@ if ((await confirmOverwrite(expectedPath)) !== true) { await workflowStore.saveWorkflow(workflow) - return + return true } await deleteWorkflow(existing, true) } @@ } await workflowStore.saveWorkflow(workflow) + return true } }// caller example (outside this line range) const saved = await workflowService.saveWorkflow(workflow) if (saved) close()As per coding guidelines:
src/**/*.{js,ts,vue}: Implement proper error propagation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/workflow/core/services/workflowService.ts` around lines 157 - 191, saveWorkflow currently returns void so callers can't detect a user-cancel from saveWorkflowAs; change saveWorkflow to return a boolean (true on successful save, false on cancel) by capturing the result of saveWorkflowAs(workflow) when workflow.isTemporary and returning false if that call returns false, and return true after successful flows in the non-temporary branch (after renameWorkflow/workflowStore.saveWorkflow completes); ensure all internal branches that short-circuit (e.g., the confirmOverwrite path where you call workflowStore.saveWorkflow and return) return an appropriate boolean, and update any callers of saveWorkflow to await and handle the boolean result.
🧹 Nitpick comments (3)
src/components/builder/DefaultViewDialogContent.vue (1)
80-93: Consider makingviewTypeOptionsreactive for locale changes.The
viewTypeOptionsarray is defined once at component setup usingt(). If the user changes their locale while the dialog is open, these translated strings won't update. This is a minor edge case, but for consistency with other components, consider using a computed property.♻️ Optional: Make viewTypeOptions a computed
-const viewTypeOptions = [ +const viewTypeOptions = computed(() => [ { value: true, icon: 'icon-[lucide--app-window]', title: t('builderToolbar.app'), subtitle: t('builderToolbar.appDescription') }, { value: false, icon: 'icon-[comfy--workflow]', title: t('builderToolbar.nodeGraph'), subtitle: t('builderToolbar.nodeGraphDescription') } -] +])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/builder/DefaultViewDialogContent.vue` around lines 80 - 93, The viewTypeOptions array is built once using t() so translations won't update on locale change; change viewTypeOptions from a static constant to a computed property (e.g., computed(() => [ ... ]) ) that calls t() for title/subtitle so it reacts to i18n locale updates; update any references to viewTypeOptions in the template or setup to use the new computed ref, preserving the same value/icon structure.packages/shared-frontend-utils/src/formatUtil.ts (1)
42-46: Consider documenting thegetWorkflowSuffixfallback behavior.The function silently falls back to
'json'for any value other than'app.json'. While this is likely intentional, a brief JSDoc comment would clarify the design decision for future maintainers.📝 Optional: Add JSDoc for clarity
+/** + * Maps a suffix string to a valid WorkflowSuffix. + * Returns 'app.json' only for exact match; all other values default to 'json'. + */ export function getWorkflowSuffix( suffix: string | null | undefined ): WorkflowSuffix { return suffix === 'app.json' ? 'app.json' : 'json' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-frontend-utils/src/formatUtil.ts` around lines 42 - 46, Add a short JSDoc comment above getWorkflowSuffix explaining that the function returns 'app.json' only when the input exactly equals 'app.json' and otherwise silently falls back to 'json' (covering null/undefined and any other strings), and mention the intended return type WorkflowSuffix so future maintainers understand the fallback behavior and accepted inputs.src/components/builder/useAppSetDefaultView.ts (1)
49-56: Minor:reset()is called twice when applying.When
handleApplycallscloseDialog(), thecloseDialogfunction callsreset(). This is fine since settingsettingView.value = falsetwice is idempotent, but you could simplify by removingreset()fromcloseDialogif it's always paired with explicit reset scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/builder/useAppSetDefaultView.ts` around lines 49 - 56, The closeDialog function currently calls reset(), causing reset() to be invoked redundantly when handleApply calls closeDialog and then also calls reset() — remove the reset() call from closeDialog so that closing the dialog only calls dialogStore.closeDialog({ key: DIALOG_KEY }), and ensure callers that need to clear state (e.g., handleApply and any other places relying on reset) call reset() explicitly to preserve existing behavior for settingView.value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/platform/workflow/core/services/workflowService.ts`:
- Around line 157-191: saveWorkflow currently returns void so callers can't
detect a user-cancel from saveWorkflowAs; change saveWorkflow to return a
boolean (true on successful save, false on cancel) by capturing the result of
saveWorkflowAs(workflow) when workflow.isTemporary and returning false if that
call returns false, and return true after successful flows in the non-temporary
branch (after renameWorkflow/workflowStore.saveWorkflow completes); ensure all
internal branches that short-circuit (e.g., the confirmOverwrite path where you
call workflowStore.saveWorkflow and return) return an appropriate boolean, and
update any callers of saveWorkflow to await and handle the boolean result.
---
Nitpick comments:
In `@packages/shared-frontend-utils/src/formatUtil.ts`:
- Around line 42-46: Add a short JSDoc comment above getWorkflowSuffix
explaining that the function returns 'app.json' only when the input exactly
equals 'app.json' and otherwise silently falls back to 'json' (covering
null/undefined and any other strings), and mention the intended return type
WorkflowSuffix so future maintainers understand the fallback behavior and
accepted inputs.
In `@src/components/builder/DefaultViewDialogContent.vue`:
- Around line 80-93: The viewTypeOptions array is built once using t() so
translations won't update on locale change; change viewTypeOptions from a static
constant to a computed property (e.g., computed(() => [ ... ]) ) that calls t()
for title/subtitle so it reacts to i18n locale updates; update any references to
viewTypeOptions in the template or setup to use the new computed ref, preserving
the same value/icon structure.
In `@src/components/builder/useAppSetDefaultView.ts`:
- Around line 49-56: The closeDialog function currently calls reset(), causing
reset() to be invoked redundantly when handleApply calls closeDialog and then
also calls reset() — remove the reset() call from closeDialog so that closing
the dialog only calls dialogStore.closeDialog({ key: DIALOG_KEY }), and ensure
callers that need to clear state (e.g., handleApply and any other places relying
on reset) call reset() explicitly to preserve existing behavior for
settingView.value.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/shared-frontend-utils/src/formatUtil.test.tspackages/shared-frontend-utils/src/formatUtil.tssrc/components/breadcrumb/SubgraphBreadcrumbItem.vuesrc/components/builder/BuilderMenu.vuesrc/components/builder/BuilderSaveSuccessDialogContent.vuesrc/components/builder/BuilderToolbar.vuesrc/components/builder/DefaultViewDialogContent.vuesrc/components/builder/useAppSetDefaultView.test.tssrc/components/builder/useAppSetDefaultView.tssrc/components/builder/useBuilderSave.tssrc/components/sidebar/tabs/WorkflowsSidebarTab.vuesrc/composables/useCoreCommands.tssrc/locales/en/main.jsonsrc/platform/workflow/core/services/workflowService.test.tssrc/platform/workflow/core/services/workflowService.tssrc/platform/workflow/management/stores/comfyWorkflow.tssrc/stores/appModeStore.test.tssrc/stores/appModeStore.ts
💤 Files with no reviewable changes (2)
- src/components/builder/BuilderSaveSuccessDialogContent.vue
- src/components/builder/useBuilderSave.ts
dosubot
bot
added
the
size:XXL
AustinMroz
reviewed
Mar 2, 2026
Collaborator
AustinMroz
left a comment
AustinMroz
left a comment
There was a problem hiding this comment.
This one should really have a lot more deletions. Shouldn't syncLinearMode be removed entirely?
| } | ||
|
|
||
| export function appendWorkflowJsonExt(path: string, isApp: boolean): string { | ||
| return ensureWorkflowSuffix(path, isApp ? 'app.json' : 'json') |
Collaborator
There was a problem hiding this comment.
Not blocking, but the disparity of when, and how infrequently the EXT constants are used isn't great.
Member
Author
There was a problem hiding this comment.
added consts for JSON and APP_JSON for the non dot-prefixed ones
| } | ||
| } | ||
| const dotIndex = fullFilename.lastIndexOf('.') | ||
| if (dotIndex > 0) { |
Collaborator
There was a problem hiding this comment.
Can invert the conditional to further polish to
Suggested change
| if (dotIndex > 0) { | |
| if (dotIndex <= 0) return { filename: fullFilename, suffix: null } |
but I'm happy enough with the current performance improvements.
| initialOpenAsApp?: boolean | ||
| }>() | ||
|
|
||
| const emit = defineEmits<{ |
Collaborator
There was a problem hiding this comment.
Suggested change
| const emit = defineEmits<{ | |
| defineEmits<{ |
Also very nit, you can do an empty defineEmits and use $emit in the template
| const dialogService = useDialogService() | ||
| const dialogStore = useDialogStore() | ||
|
|
||
| const settingView = ref(false) |
Collaborator
There was a problem hiding this comment.
Seems kinda smelly to be reimplementing the visibility guards here when dialogStore/dialogService already check for duplicate dialogues through the group key and provides a isDialogOpen
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared-frontend-utils/src/formatUtil.ts`:
- Around line 137-141: The .app.json special-case returns an empty filename when
there is no basename; update the branch that checks lower.endsWith(APP_JSON_EXT)
to only treat it as a suffix if there is a real basename (e.g., ensure dotIndex
> 0 or fullFilename has characters before the dot) — otherwise fall through to
the existing dotfile handling. Modify the logic around APP_JSON_EXT/APP_JSON and
fullFilename so you compute dotIndex (or reuse the existing dotIndex variable)
and require dotIndex > 0 before returning { filename: fullFilename.slice(0,
-APP_JSON_EXT.length), suffix: APP_JSON }.
In `@src/platform/workflow/core/services/workflowService.ts`:
- Around line 130-133: The current flow deletes existingWorkflow via
deleteWorkflow when isSelfOverwrite is false before the replacement is durably
saved; change to a save-first-then-delete or transactional replace: write the
new workflow to a temporary/alternate key or use a persistence-level
atomic/overwrite API (e.g. persistWorkflow/saveWorkflow/replaceWorkflow) and
only call deleteWorkflow(existingWorkflow, true) after the new content has been
successfully persisted and committed; ensure any write or commit errors abort
the delete, propagate errors up (do not swallow), and add proper
try/catch/finally around the save+delete sequence so failures leave the original
workflow intact.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/shared-frontend-utils/src/formatUtil.tssrc/components/builder/BuilderToolbar.vuesrc/components/builder/DefaultViewDialogContent.vuesrc/components/builder/useAppSetDefaultView.test.tssrc/components/builder/useAppSetDefaultView.tssrc/platform/workflow/core/services/workflowService.test.tssrc/platform/workflow/core/services/workflowService.tssrc/platform/workflow/management/stores/comfyWorkflow.ts
💤 Files with no reviewable changes (1)
- src/platform/workflow/management/stores/comfyWorkflow.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/builder/useAppSetDefaultView.test.ts
- src/components/builder/useAppSetDefaultView.ts
Comment on lines
130
to
133
| if (!isSelfOverwrite) { | ||
| const deleted = await deleteWorkflow(existingWorkflow, true) | ||
| if (!deleted) return false | ||
| } |
Contributor
There was a problem hiding this comment.
Delete-before-save ordering risks destructive overwrite failures.
Line 131 and Line 173 remove the existing target workflow before the
replacement is durably saved. If a later rename/open/save step fails, the old
workflow is already gone.
Please switch this flow to an atomic/transactional replacement strategy (or an
explicit overwrite API in persistence) so deletion only occurs after successful
write of the new content.
As per coding guidelines: "Implement proper error handling" and "Implement proper error propagation".
Also applies to: 168-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/workflow/core/services/workflowService.ts` around lines 130 -
133, The current flow deletes existingWorkflow via deleteWorkflow when
isSelfOverwrite is false before the replacement is durably saved; change to a
save-first-then-delete or transactional replace: write the new workflow to a
temporary/alternate key or use a persistence-level atomic/overwrite API (e.g.
persistWorkflow/saveWorkflow/replaceWorkflow) and only call
deleteWorkflow(existingWorkflow, true) after the new content has been
successfully persisted and committed; ensure any write or commit errors abort
the delete, propagate errors up (do not swallow), and add proper
try/catch/finally around the save+delete sequence so failures leave the original
workflow intact.
Member
Author
There was a problem hiding this comment.
This is a pre-existing pattern for workflow overwriting, however you are correct that we should clean this up, but not in this PR. Can you add a new issue for resolving this, probably by allowing for the overwrite flag to be passed through when saving the workflow
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@pythongosssss, understood! I'll create a follow-up issue for implementing atomic workflow overwrite.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for atomic workflow overwrite
gh issue create \
--title "Implement atomic workflow overwrite via persistence-level flag" \
--body "## Problem
The current workflow save/overwrite flow deletes the existing workflow before the replacement is durably saved (e.g., in \`saveWorkflowAs\` and \`saveWorkflow\`). If a subsequent rename, open, or save step fails, the original workflow is already gone, risking data loss.
Affected locations:
- \`src/platform/workflow/core/services/workflowService.ts\` Line 131 (in \`saveWorkflowAs\`)
- \`src/platform/workflow/core/services/workflowService.ts\` Line 173 (in \`saveWorkflow\`)
## Proposed Solution
Add an \`overwrite\` flag (or similar) to the workflow persistence layer (\`saveWorkflow\` / \`persistWorkflow\` API) that allows atomic replacement:
- Write the new workflow content first
- Only delete/replace the old workflow after successful commit
- Ensure any write failures abort the delete and propagate errors up
This would replace the current pattern of explicit \`deleteWorkflow\` calls before saving.
## References
- PR: https://github.com/Comfy-Org/ComfyUI_frontend/pull/9338
- Comment: https://github.com/Comfy-Org/ComfyUI_frontend/pull/9338#discussion_r2878008918
- Requested by: `@pythongosssss`" \
--assignee pythongosssss
Length of output: 57
@pythongosssss, done! I've created issue #9355 to track implementing atomic workflow overwrite via a persistence-level flag.
🧠 Learnings used
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 9345
File: src/platform/workflow/management/stores/workflowStore.ts:488-491
Timestamp: 2026-03-03T01:32:49.646Z
Learning: In `src/platform/workflow/management/stores/workflowStore.ts`, `openWorkflowPaths` maintains unique workflow paths by design. Operations like `openWorkflowsInBackground` use `_.union` to enforce uniqueness. When updating paths (e.g., during rename), replacing the first occurrence with `indexOf`/`splice` is sufficient because duplicates should not exist.
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-23T21:34:45.361Z
Learning: Development workflow: make code changes, run relevant tests, run `pnpm typecheck`, `pnpm lint`, `pnpm format`, check if README updates are needed, suggest docs.comfy.org updates for user-facing changes
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7169
File: src/platform/remote/comfyui/jobs/jobTypes.ts:1-107
Timestamp: 2025-12-09T03:39:54.501Z
Learning: In the ComfyUI_frontend project, Zod is on v3.x. Do not suggest Zod v4 standalone validators (z.uuid, z.ulid, z.cuid2, z.nanoid) until an upgrade to Zod 4 is performed. When reviewing TypeScript files (e.g., src/platform/remote/comfyui/jobs/jobTypes.ts) validate against Zod 3 capabilities and avoid introducing v4-specific features; flag any proposal to upgrade or incorporate v4-only validators and propose staying with compatible 3.x patterns.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7358
File: src/components/dialog/content/signin/SignUpForm.vue:45-54
Timestamp: 2025-12-11T12:25:15.470Z
Learning: This repository uses CI automation to format code (pnpm format). Do not include manual formatting suggestions in code reviews for Comfy-Org/ComfyUI_frontend. If formatting issues are detected, rely on the CI formatter or re-run pnpm format. Focus reviews on correctness, readability, performance, accessibility, and maintainability rather than style formatting.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7416
File: src/stores/imagePreviewStore.ts:5-7
Timestamp: 2025-12-13T11:03:11.264Z
Learning: In the ComfyUI_frontend repository, lint rules require keeping 'import type' statements separate from non-type imports, even if importing from the same module. Do not suggest consolidating them into a single import statement. Ensure type imports remain on their own line (import type { ... } from 'module') and regular imports stay on separate lines.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7537
File: src/components/ui/button/Button.stories.ts:45-55
Timestamp: 2025-12-17T00:40:09.635Z
Learning: Prefer pure function declarations over function expressions (e.g., use function foo() { ... } instead of const foo = () => { ... }) for pure functions in the repository. Function declarations are more functional-leaning, offer better hoisting clarity, and can improve readability and tooling consistency. Apply this guideline across TypeScript files in Comfy-Org/ComfyUI_frontend, including story and UI component code, except where a function expression is semantically required (e.g., callbacks, higher-order functions with closures).
Learnt from: kaili-yang
Repo: Comfy-Org/ComfyUI_frontend PR: 7805
File: src/composables/useCoreCommands.ts:439-439
Timestamp: 2025-12-30T22:22:33.836Z
Learning: When accessing reactive properties from Pinia stores in TypeScript files, avoid using .value on direct property access (e.g., useStore().isOverlayExpanded). Pinia auto-wraps refs when accessed directly, returning the primitive value. The .value accessor is only needed when destructuring store properties or when using storeToRefs().
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7906
File: src/components/sidebar/tabs/AssetsSidebarTab.vue:545-552
Timestamp: 2026-01-12T17:39:27.738Z
Learning: In Vue/TypeScript files (src/**/*.{ts,tsx,vue}), prefer if/else statements over ternary operators when performing side effects or actions (e.g., mutating state, calling methods with side effects). Ternaries should be reserved for computing and returning values.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8248
File: src/platform/assets/schemas/assetSchema.ts:109-118
Timestamp: 2026-01-22T23:44:59.888Z
Learning: Do not export declarations (e.g., schemas, constants, types) unless they are actually used elsewhere in the codebase. Keep the public API surface minimal by exporting only items with concrete usage. This reduces coupling and keeps imports explicit. In src/platform/assets/schemas/assetSchema.ts, ensure only necessary exports are exposed; remove unused exports or switch to non-exported/internal where appropriate.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8722
File: src/composables/maskeditor/useBrushDrawing.ts:5-5
Timestamp: 2026-02-08T03:19:12.009Z
Learning: Treat unresolved import warnings caused by pnpm workspace catalog mappings (pnpm-workspace.yaml) and unplugin-typegpu in vite.config.mts as false positives. Do not flag these in PR reviews; they are expected in sandboxes without node_modules and are not indicative of actual missing dependencies in the TS codebase.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 8966
File: src/extensions/core/load3d.ts:427-430
Timestamp: 2026-02-19T02:06:23.468Z
Learning: In TypeScript, you can use typeof SomeClass in type annotation positions (e.g., param: typeof LGraphNode) even when SomeClass is imported with import type. This is a type query that only exists at compile time and is erased in runtime, so it is safe to combine with type-only imports. Apply this pattern to TS files broadly when you need a type that references the shape of a class or constructor function without importing the value at runtime.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 8966
File: src/extensions/core/uploadAudio.ts:91-94
Timestamp: 2026-02-19T02:06:38.395Z
Learning: In TypeScript files, you can use a type annotation like 'nodeType: typeof MyClass' even if MyClass is imported via 'import type'. Both the type-only import and 'typeof' operate at the type level and are erased at compile time. This pattern is commonly used for constructor types (e.g., 'nodeType: typeof LGraphNode'). Apply this pattern across TypeScript files in the repository (src/**/*.ts) as appropriate, ensuring the imported symbol is a type-only import when possible for clarity and to avoid runtime imports.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8992
File: src/lib/litegraph/src/widgets/GradientSliderWidget.ts:18-18
Timestamp: 2026-02-20T21:08:19.814Z
Learning: When drawing with CanvasRenderingContext2D in TypeScript/JavaScript, wrap the drawing logic with ctx.save() at the start and ctx.restore() at the end to preserve and restore the canvas state. Do not manually destructure and restore individual properties (e.g., fillStyle, strokeStyle); rely on save/restore to manage state changes in a scoped manner. This should be applied to all TS files that perform canvas drawing.
Learnt from: dante01yoon
Repo: Comfy-Org/ComfyUI_frontend PR: 9075
File: src/scripts/api.featureFlags.test.ts:237-268
Timestamp: 2026-02-22T04:27:33.379Z
Learning: In Vite/Vitest, import.meta.env.DEV is true for any mode that is not 'production' (i.e., DEV is the opposite of PROD, and can be true in 'test', 'development', etc.). Do not assume DEV implies only 'development' mode. When reviewing code and tests, treat DEV as a non-production flag and verify environment-specific logic accordingly. Reference: https://vite.dev/guide/env-and-mode#modes
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/shared-frontend-utils/src/formatUtil.ts (1)
137-141:⚠️ Potential issue | 🟡 MinorGuard
.app.jsonspecial-case when basename is emptyOn Line 137,
.app.jsonis classified as{ filename: '', suffix: 'app.json' }instead of falling through to dotfile handling. That can leak an empty filename into rename/save paths.Suggested fix
export function getFilenameDetails(fullFilename: string) { const lower = fullFilename.toLowerCase() if (lower.endsWith(APP_JSON_EXT)) { + const filename = fullFilename.slice(0, -APP_JSON_EXT.length) + if (filename.length > 0) { return { - filename: fullFilename.slice(0, -APP_JSON_EXT.length), - suffix: APP_JSON + filename, + suffix: APP_JSON + } } } const dotIndex = fullFilename.lastIndexOf('.')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-frontend-utils/src/formatUtil.ts` around lines 137 - 141, The .app.json branch incorrectly returns { filename: '', suffix: APP_JSON } for inputs like ".app.json"; update the guard in the block that checks lower.endsWith(APP_JSON_EXT) (using symbols APP_JSON_EXT and APP_JSON and the existing fullFilename slice) to compute const base = fullFilename.slice(0, -APP_JSON_EXT.length) and only return the special-case when base !== '' (otherwise fall through to the dotfile handling); ensure the code uses base for the filename in the returned object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/shared-frontend-utils/src/formatUtil.ts`:
- Around line 137-141: The .app.json branch incorrectly returns { filename: '',
suffix: APP_JSON } for inputs like ".app.json"; update the guard in the block
that checks lower.endsWith(APP_JSON_EXT) (using symbols APP_JSON_EXT and
APP_JSON and the existing fullFilename slice) to compute const base =
fullFilename.slice(0, -APP_JSON_EXT.length) and only return the special-case
when base !== '' (otherwise fall through to the dotfile handling); ensure the
code uses base for the filename in the returned object.
fix .app.json file giving empty filename
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shared-frontend-utils/src/formatUtil.ts (1)
44-48: MakegetWorkflowSuffixcomparison case-insensitive for defensive inputs.If a caller passes
'APP.JSON', this currently falls back to'json'.
A lowercase-normalized compare makes this utility safer as a public API.Suggested patch
export function getWorkflowSuffix( suffix: string | null | undefined ): WorkflowSuffix { - return suffix === APP_JSON_SUFFIX ? APP_JSON_SUFFIX : JSON_SUFFIX + return suffix?.toLowerCase() === APP_JSON_SUFFIX + ? APP_JSON_SUFFIX + : JSON_SUFFIX }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-frontend-utils/src/formatUtil.ts` around lines 44 - 48, getWorkflowSuffix currently compares suffix to APP_JSON_SUFFIX case-sensitively; normalize suffix (e.g., const normalized = suffix?.toLowerCase()) and compare normalized to APP_JSON_SUFFIX.toLowerCase() so inputs like 'APP.JSON' match; keep null/undefined handling and still return APP_JSON_SUFFIX or JSON_SUFFIX as before, referencing getWorkflowSuffix, APP_JSON_SUFFIX, and JSON_SUFFIX when you update the comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/shared-frontend-utils/src/formatUtil.ts`:
- Around line 44-48: getWorkflowSuffix currently compares suffix to
APP_JSON_SUFFIX case-sensitively; normalize suffix (e.g., const normalized =
suffix?.toLowerCase()) and compare normalized to APP_JSON_SUFFIX.toLowerCase()
so inputs like 'APP.JSON' match; keep null/undefined handling and still return
APP_JSON_SUFFIX or JSON_SUFFIX as before, referencing getWorkflowSuffix,
APP_JSON_SUFFIX, and JSON_SUFFIX when you update the comparison.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/shared-frontend-utils/src/formatUtil.test.tspackages/shared-frontend-utils/src/formatUtil.tssrc/components/builder/useAppSetDefaultView.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/builder/useAppSetDefaultView.ts
- packages/shared-frontend-utils/src/formatUtil.test.ts
AustinMroz
approved these changes
Mar 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Change app mode changes to be written directly to the workflow on change instead of requiring explicit save via builder.
Temporary: Adds
.app.jsonfile extension to app files for identification since we don't currently have a way to identify them with metadataRemoves app builder save dialog and replaces it with default mode selection
Changes
What:
ensure all save locations handle app mode
remove dirtyLinearData and flushing
Breaking:
if people are relying on workflow names and are converting to/from app mode in the same workflow, they will gain/lose the
.apppart of the extensionScreenshots (if applicable)
┆Issue is synchronized with this Notion page by Unito